Accept label value as regexp #171

sathieu · 2023-12-07T10:46:19Z

Fixes: #82

simonpasquier

Thanks for tackling this! Reading the issue, I have the impression that supporting regexp label values only for --label-value was enough (e.g. only for static values). Do you have a use case for supporting the same for --query-param and --header-name?

Given the security implications (e.g. it's easy to get the regexp wrong and expose too many metrics by accident), I'd be more inclined to limit the scope for now.

README.md

main.go

sathieu · 2023-12-07T15:05:32Z

Thanks for tackling this! Reading the issue, I have the impression that supporting regexp label values only for --label-value was enough (e.g. only for static values). Do you have a use case for supporting the same for --query-param and --header-name?

Yes.

Here is my usecase : I have multiple (soft-)tenants in a Kubernetes cluster. Each can include a list of namespace ([app1, app2]) or a list of namespace regexps ([tenant1-.+]). For the first case, Grafana is not able to pass the same header multiple times (see grafana/grafana-plugin-sdk-go#829), so here regexp support is a workaround. For the second case (less common), I really need regexp support.

Alternatively, a new tenant label could have been set at scrapping time. This is waiting for Scrape classes support in Prometheus Operator.

Given the security implications (e.g. it's easy to get the regexp wrong and expose too many metrics by accident), I'd be more inclined to limit the scope for now.

I understand. But I need it 😉 . Maybe add a warning in the docs?

simonpasquier · 2023-12-07T20:35:59Z

Thanks for the details, it helps a lot! I could agree to the feature being marked as experimental with an very clear warning in the CLI help + README.md file.

The proxy needs also to validate that the regexp is valid. Also what should the proxy's response when it gets several label values? My gut feeling is that it should fail.

sathieu · 2023-12-11T13:45:23Z

Thanks @simonpasquier I've rebased, added warnings in README, added checks for invalid regex and multiple values.

Back to you 🏓 !

README.md

simonpasquier · 2023-12-11T15:38:16Z

README.md

+   -header-name X-Namespace \
+   -label namespace \


(suggestion) I'd use a static label value to make it more "visible".

Suggested change

-header-name X-Namespace \

-label namespace \

-label-value '^foo-.+$' \

-label namespace \

I wonder if we shouldn't anchor the regexp by default (like Prometheus does for relabelings)? It would at least prevent too-permissive configurations.
It'd also be good to require that the compiled regex doesn't match the empty string.

I wonder if we shouldn't anchor the regexp by default (like Prometheus does for relabelings)? It would at least prevent too-permissive configurations.

Prometheus anchors already.

It'd also be good to require that the compiled regex doesn't match the empty string.

Good idea.

main.go

injectproxy/routes.go

sathieu · 2023-12-11T17:02:17Z

@simonpasquier I've merged your suggestions, added tests for regexp matching empty strings, and added tests early when using -label-value.

main.go

injectproxy/silences_test.go

injectproxy/routes_test.go

sathieu · 2023-12-12T15:35:44Z

@simonpasquier Thanks for your review. I've merged your suggestions, and silence API is now returning 501 Not Implemented when regex match is enabled. I'll try to add support for multiple values in silences in a future PR.

Back to you 🏸 !

simonpasquier

one last comment...

simonpasquier · 2023-12-20T15:21:05Z

injectproxy/routes.go

-		mux.Handle("/api/v2/silences", r.el.ExtractLabel(
-			enforceMethods(
-				assertSingleLabelValue(r.silences),
+	if opt.regexMatch {


(suggestion) to avoid duplicating the mux.Handle() calls, we might do something like this:

errs.Add( mux.Handle("/api/v2/silences", r.errorIfRegexpMatch( r.el.ExtractLabel(... [...] func (r *routes) errorIfRegexpMatch(next http.HandlerFunc) http.HandlerFunc { return func(w http.ResponseWriter, req *http.Request) { if r.regexMatch { prometheusAPIError(w, "support for regex match not implemented", http.StatusNotImplemented) return } return next(w, req) } }

Done @simonpasquier. Back to you ⚾ !

simonpasquier

Thanks! 👍 for me.
@squat do you have any concern with this addition? From my side, the warning in the README.md is clear enough that users know about the potential issues if they mess up with their regexp.

README.md

Fixes: prometheus-community#82 Signed-off-by: Mathieu Parent <[email protected]>

PR prometheus-community#171 implemented support for regex label values but only for the query endpoints. This change adds support for all other Prometheus API endpoints. Signed-off-by: Simon Pasquier <[email protected]>

sathieu mentioned this pull request Dec 7, 2023

Specify label value in prom-label-proxy config / Wildcards in Labels #82

Closed

sathieu force-pushed the label_regexp branch from 5ece26e to e4668d0 Compare December 7, 2023 12:41

simonpasquier reviewed Dec 7, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

main.go Outdated Show resolved Hide resolved

sathieu force-pushed the label_regexp branch from e4668d0 to 6c387f7 Compare December 7, 2023 14:51

sathieu force-pushed the label_regexp branch from 6c387f7 to 9d4dfd7 Compare December 11, 2023 13:38

simonpasquier reviewed Dec 11, 2023

View reviewed changes

sathieu force-pushed the label_regexp branch 3 times, most recently from 423ccc9 to 372db15 Compare December 11, 2023 17:01

simonpasquier reviewed Dec 12, 2023

View reviewed changes

main.go Show resolved Hide resolved

injectproxy/silences_test.go Show resolved Hide resolved

injectproxy/routes_test.go Outdated Show resolved Hide resolved

injectproxy/routes_test.go Outdated Show resolved Hide resolved

injectproxy/routes_test.go Outdated Show resolved Hide resolved

sathieu force-pushed the label_regexp branch 2 times, most recently from 646a9f5 to 791471e Compare December 12, 2023 15:33

sathieu requested a review from simonpasquier December 19, 2023 15:16

simonpasquier reviewed Dec 20, 2023

View reviewed changes

sathieu requested a review from simonpasquier December 21, 2023 09:25

sathieu mentioned this pull request Dec 21, 2023

Added support for regex label #100

Closed

sathieu force-pushed the label_regexp branch from 791471e to 1f6a4d9 Compare December 21, 2023 10:07

simonpasquier reviewed Dec 21, 2023

View reviewed changes

README.md Outdated Show resolved Hide resolved

Accept label value as regexp

779c3bd

Fixes: prometheus-community#82 Signed-off-by: Mathieu Parent <[email protected]>

sathieu force-pushed the label_regexp branch from 1f6a4d9 to 779c3bd Compare December 21, 2023 16:11

simonpasquier approved these changes Dec 22, 2023

View reviewed changes

simonpasquier merged commit f37112a into prometheus-community:main Jan 2, 2024
4 checks passed

simonpasquier mentioned this pull request Jun 7, 2024

fix: support regex for non-query endpoints #226

Merged

simonpasquier mentioned this pull request Jun 13, 2024

prom-label-proxy refusing to proxy for Grafana even if the required label is specified #74

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept label value as regexp #171

Accept label value as regexp #171

sathieu commented Dec 7, 2023

simonpasquier left a comment

sathieu commented Dec 7, 2023

simonpasquier commented Dec 7, 2023

sathieu commented Dec 11, 2023 •

edited

Loading

simonpasquier Dec 11, 2023

simonpasquier Dec 11, 2023

sathieu Dec 11, 2023

sathieu commented Dec 11, 2023

sathieu commented Dec 12, 2023

simonpasquier left a comment

simonpasquier Dec 20, 2023

sathieu Dec 21, 2023

simonpasquier left a comment

Accept label value as regexp #171

Accept label value as regexp #171

Conversation

sathieu commented Dec 7, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

sathieu commented Dec 7, 2023

simonpasquier commented Dec 7, 2023

sathieu commented Dec 11, 2023 • edited Loading

simonpasquier Dec 11, 2023

Choose a reason for hiding this comment

simonpasquier Dec 11, 2023

Choose a reason for hiding this comment

sathieu Dec 11, 2023

Choose a reason for hiding this comment

sathieu commented Dec 11, 2023

sathieu commented Dec 12, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

simonpasquier Dec 20, 2023

Choose a reason for hiding this comment

sathieu Dec 21, 2023

Choose a reason for hiding this comment

simonpasquier left a comment

Choose a reason for hiding this comment

sathieu commented Dec 11, 2023 •

edited

Loading